Query bank balances #51

samsiegart · 2023-01-26T19:52:44Z

refs #49

We still need to update dapp-inter to do the same, then I can test e2e with amounts in offer proposals rather than pursepetname+value

cloudflare-workers-and-pages · 2023-01-26T19:55:19Z

Deploying with Cloudflare Pages

Latest commit:	`d312e60`
Status:	✅ Deploy successful!
Preview URL:	https://b2b65e05.wallet-app.pages.dev
Branch Preview URL:	https://query-bank.wallet-app.pages.dev

View logs

samsiegart · 2023-01-26T19:57:48Z

src/util/WalletBackendAdapter.ts

+ );
+
+ vbankAssets.forEach(([denom, info]) => {
+ const amount = bankMap.get(denom) ?? 0n;


If an asset exists in the vbank but not in the user's account we want to show it as a purse with 0 balance so they can click the eventual "deposit" button for ibc transfer.

Very helpful note. Please include in the code.

dckc

Looks plausible.

I'd like to see test coverage for this feature, as usual. Skipping it for now is OK with me if it's OK with @turadg .

dckc · 2023-01-26T20:26:28Z

src/service/Offers.ts

+ /// XXX test e2e with dapp inter once feasible.
+ const amount = serializedAmount
+ ? await E(boardIdMarshaller).unserialize(serializedAmount)
+ : { brand: pursePetnameToBrand.get(pursePetname), value };


we're cool with open-coding AmountMath.make()? maybe the notion of brand used here doesn't quite line up?

Thanks, you caught a bug, I wasn't converting value to BigInt. Just changed to use AmountMath and confirmed it works.

dckc · 2023-01-26T20:29:55Z

src/util/queryBankBalances.ts

+ address: string,
+ rpc: HttpEndpoint,
+): Promise<Coin[]> => {
+ const tendermint = await Tendermint34Client.connect(rpc);


ambient authority. hm.

No modules should export powerful objects (for example, objects that close over fs or process.env). -- https:/Agoric/agoric-sdk/wiki/OCap-Discipline

I suppose we have agreed on an exception to that for UI code? I'd like to see that in the README or the like.

Noted, I don't know if I have a clear way to explain the philosophy as it pertains to UI code at the moment though.

I think we have agreed agreed on an exception to that for UI code and I agree it would be valuable to have in the README of this repo to refer to. Not a blocker imo

agreed, not a blocker.

meanwhile, I realize pointing to the wiki doesn't automatically make a link the other way, where citing an issue does: Agoric/agoric-sdk#2160

dckc · 2023-01-26T20:35:12Z

src/util/queryBankBalances.ts

+ const rpcClient = createProtobufRpcClient(queryClient);
+ const bankQueryService = new QueryClientImpl(rpcClient);
+
+ const { balances } = await bankQueryService.AllBalances({


scaling consideration: How often do we do this? I gather from @arirubinstein that we're supposed to make no more than 2 RPC queries per second.

I gather @michaelfig plans to provide rate limiting in @agoric/casting (IOU an issue #) Do we have an issue about migrating this code to use it? Cite it from a TODO?

Yea my hope is this could unblock us for now and we can swap in the improved casting stuff later, just added a comment here.

filed:

PSM dapp, wallet poll RPC nodes rapidly agoric-sdk#6870

dckc · 2023-01-26T20:37:59Z

src/util/WalletBackendAdapter.ts

+ // We only care about the zoe invite purse, which has asset kind 'set'.
+ // Non 'set' amounts need to be fetched from vbank to know their
+ // decimalPlaces.
+ //
+ // If we have add non 'set' amount purses that aren't in the vbank, it's
+ // not currently possible to read their decimalPlaces, so this code
+ // will need updating.


This documentation is good.

Ideally we'd import some invitationIssuerAssetKind constant from @agoric/zoe/something.

I leave it to @turadg to judge the cost-effectiveness of that.

I don't know of any runtime features of an invitation issuer that distinguishes it from any other Set kind issuer.

You could make this "set"-ness check more apparent using @agoric/patterns (soon to be in Endo): matches(purse.balances.value, SetValueShape).

Looks like @agoric/patterns doesn't exist yet either, but that would be cool.

Oops I meant '@agoric/store'. But yeah let's wait for it to be in Endo

turadg · 2023-01-26T22:15:23Z

src/service/Offers.ts

- const mapPursePetnamesToBrands = paymentProposals =>
- Object.fromEntries(
+ const convertProposals = async paymentProposals => {
+ const entries = await Promise.all(


this is a little hard to follow. consider filtering and naming the value transformer.

const convertProposal = ({ pursePetname, value, amount: serializedAmount }) => serializedAmount ? await E(boardIdMarshaller).unserialize(serializedAmount) : AmountMath.make( pursePetnameToBrand.get(pursePetname), BigInt(value); Object.entries(paymentProposals). filter(([_kw, { pursePetname, amount}]) => amount && pursePetnameToBrand.get(pursePetname)) .map(([kw, proposal]) => convertProposal);

Then it's a small step to using objectMap and deeplyFulfilled;

I gave this a shot but I got a little confused with how to use those utility functions and the code wasn't looking much simpler, I'd prefer to leave this as is.

turadg · 2023-01-26T22:16:34Z

src/service/Offers.ts

- },
- ];
+
+ /// XXX test e2e with dapp inter once feasible.


thanks for the documenting. consider TODO.

turadg · 2023-01-26T22:24:46Z

src/service/Offers.ts

+ const instance = await E(boardIdMarshaller).unserialize(instanceHandle);
+ const {
+ slots: [instanceBoardId],
+ } = await E(boardIdMarshaller).serialize(instance);


i had to remember there's no promise pipelining in this app so the two awaits are fine.

it wasn't obvious to me why you wanted to wrap these in a function. It looks like you want convertProposals to resolve in parallel to resolving the instance. Good idea. But I don't see why you want instanceBoardId to be available after this Promise.all.

consider,

const [instance, give, want] = await Promise.all([ E(boardIdMarshaller).unserialize(instanceHandle), convertProposals(giveTemplate), convertProposals(wantTemplate), ]);

and then just above the first use of instanceBoardId,

const { slots: [instanceBoardId], } = await E(boardIdMarshaller).serialize(instance);

Done.

I'm not sure about promise pipelining in the rest of the app, but we await here so that the card doesn't show up before it has the data needed to render.

turadg · 2023-01-26T22:26:06Z

src/util/WalletBackendAdapter.ts

 beansOwingUpdater.updateState(Number(value));
 }
 };

+ // Reads purses from the cosmos bank module. These are not necessarily real


consider "Infer purses from Cosmos bank module balances" since, as you say, purses can't be read from Cosmos.

turadg · 2023-01-26T22:27:22Z

src/util/WalletBackendAdapter.ts

+ );
+
+ vbankAssets.forEach(([denom, info]) => {
+ const amount = bankMap.get(denom) ?? 0n;


Very helpful note. Please include in the code.

turadg · 2023-01-26T22:53:44Z

src/util/WalletBackendAdapter.ts

+ // We only care about the zoe invite purse, which has asset kind 'set'.
+ // Non 'set' amounts need to be fetched from vbank to know their
+ // decimalPlaces.
+ //
+ // If we have add non 'set' amount purses that aren't in the vbank, it's
+ // not currently possible to read their decimalPlaces, so this code
+ // will need updating.


I don't know of any runtime features of an invitation issuer that distinguishes it from any other Set kind issuer.

You could make this "set"-ness check more apparent using @agoric/patterns (soon to be in Endo): matches(purse.balances.value, SetValueShape).

turadg · 2023-01-26T22:55:30Z

src/util/WalletBackendAdapter.ts

+ if (
+ !Array.isArray(purse.balance.value) ||
+ !agoricBrands.has(purse.brand)
+ ) {
+ continue;
+ }


consider distinguishing these conditions:

if (!Array.isArray(purse.balance.value)) { console.debug('skipping non-set amount', purse.balance.value); } if (!agoricBrands.has(purse.brand)) { console.debug('skipping unknown brand', purse.brand);

Likely to pay off debugging someday. And in the meantime it's code documentation.

turadg · 2023-01-26T22:57:19Z

src/util/queryBankBalances.ts

+import type { Coin } from '@cosmjs/stargate';
+import type { HttpEndpoint } from '@cosmjs/tendermint-rpc';
+
+// XXX use casting support when possible for load balancing, batching, and


Do we have a ticket to track when it's possible? I think we should. Consider noting it here with an UNTIL.

Not sure about the ticket yet, changed to UNTIL

turadg · 2023-01-26T22:58:35Z

src/util/queryBankBalances.ts

+ address: string,
+ rpc: HttpEndpoint,
+): Promise<Coin[]> => {
+ const tendermint = await Tendermint34Client.connect(rpc);


I think we have agreed agreed on an exception to that for UI code and I agree it would be valuable to have in the README of this repo to refer to. Not a blocker imo

turadg · 2023-01-26T22:59:13Z

yarn.lock

@@ -12164,7 +12164,7 @@ rollup@^2.43.1:
 optionalDependencies:
 fsevents "~2.3.2"

-rollup@endojs/endo#rollup-2.7.1-patch-1, "rollup@github:endojs/endo#rollup-2.7.1-patch-1":
+rollup@endojs/endo#rollup-2.7.1-patch-1:


turadg

🚀

samsiegart added 4 commits January 23, 2023 15:22

feat: add queryBankBalances fn

ad6530d

feat: allow brands instead of pursePetnames for offers

bb1b673

feat: read bank balances for purses

83ccf67

feat: allow serialized amounts in offer proposals

c22b225

samsiegart commented Jan 26, 2023

View reviewed changes

samsiegart requested review from turadg and dckc January 26, 2023 20:01

dckc reviewed Jan 26, 2023

View reviewed changes

samsiegart added 2 commits January 26, 2023 12:57

fix: use AmountMath make

f26e38e

chore: document bank query todo

3fbf3fd

turadg reviewed Jan 26, 2023

View reviewed changes

samsiegart added 2 commits January 26, 2023 15:56

chore: fix some types

38c2ea2

fix: code cleanup and feedback

d312e60

samsiegart requested review from turadg and dckc January 27, 2023 01:01

turadg approved these changes Jan 27, 2023

View reviewed changes

samsiegart merged commit add681f into main Jan 27, 2023

samsiegart deleted the query-bank branch January 27, 2023 04:30

dckc mentioned this pull request Jan 27, 2023

feat(web-components): read purses from bank instead of smart wallet Agoric/agoric-sdk#6861

Merged

Query bank balances #51

Query bank balances #51

Conversation

samsiegart commented Jan 26, 2023

cloudflare-workers-and-pages bot commented Jan 26, 2023 • edited Loading

Deploying with Cloudflare Pages

samsiegart Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Jan 26, 2023 •

edited

Loading

samsiegart Jan 26, 2023 •

edited

Loading